Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AArch64] ORRWrs is copy instruction when there's no implicit def of the X register #75184

Merged
merged 7 commits into from
Dec 14, 2023

Conversation

DianQK
Copy link
Member

@DianQK DianQK commented Dec 12, 2023

Follows #74682 (comment).
Fixes #74680.

@DianQK DianQK marked this pull request as ready for review December 13, 2023 15:06
@DianQK DianQK requested review from arsenm and fpetrogalli and removed request for arsenm December 13, 2023 15:06
@llvmbot
Copy link
Member

llvmbot commented Dec 13, 2023

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-backend-aarch64

Author: DianQK (DianQK)

Changes

Follows #74682 (comment).
Fixes #74680.


Full diff: https://github.com/llvm/llvm-project/pull/75184.diff

6 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/TargetInstrInfo.h (+13)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp (+2-2)
  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+30-8)
  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.h (+2)
  • (added) llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir (+33)
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 2bbe430dc68d90..a113100f04e621 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -1025,6 +1025,11 @@ class TargetInstrInfo : public MCInstrInfo {
     return std::nullopt;
   }
 
+  virtual std::optional<DestSourcePair>
+  isCopyLikeInstrImpl(const MachineInstr &MI) const {
+    return std::nullopt;
+  }
+
   /// Return true if the given terminator MI is not expected to spill. This
   /// sets the live interval as not spillable and adjusts phi node lowering to
   /// not introduce copies after the terminator. Use with care, these are
@@ -1050,6 +1055,14 @@ class TargetInstrInfo : public MCInstrInfo {
     return isCopyInstrImpl(MI);
   }
 
+  // Similar to `isCopyInstr`, but adds non-copy semantics on MIR, but
+  // ultimately generates a copy instruction.
+  std::optional<DestSourcePair> isCopyLikeInstr(const MachineInstr &MI) const {
+    if (auto IsCopyInstr = isCopyInstr(MI))
+      return IsCopyInstr;
+    return isCopyLikeInstrImpl(MI);
+  }
+
   bool isFullCopyInstr(const MachineInstr &MI) const {
     auto DestSrc = isCopyInstr(MI);
     if (!DestSrc)
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index b2c2b40139eda6..8a78a51f72ebc5 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -2116,7 +2116,7 @@ bool InstrRefBasedLDV::transferSpillOrRestoreInst(MachineInstr &MI) {
 }
 
 bool InstrRefBasedLDV::transferRegisterCopy(MachineInstr &MI) {
-  auto DestSrc = TII->isCopyInstr(MI);
+  auto DestSrc = TII->isCopyLikeInstr(MI);
   if (!DestSrc)
     return false;
 
diff --git a/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
index 116c6b7e2d19ef..bf730be00a9a92 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
@@ -1364,7 +1364,7 @@ void VarLocBasedLDV::removeEntryValue(const MachineInstr &MI,
     // TODO: Try to keep tracking of an entry value if we encounter a propagated
     // DBG_VALUE describing the copy of the entry value. (Propagated entry value
     // does not indicate the parameter modification.)
-    auto DestSrc = TII->isCopyInstr(*TransferInst);
+    auto DestSrc = TII->isCopyLikeInstr(*TransferInst);
     if (DestSrc) {
       const MachineOperand *SrcRegOp, *DestRegOp;
       SrcRegOp = DestSrc->Source;
@@ -1840,7 +1840,7 @@ void VarLocBasedLDV::transferRegisterCopy(MachineInstr &MI,
                                            OpenRangesSet &OpenRanges,
                                            VarLocMap &VarLocIDs,
                                            TransferMap &Transfers) {
-  auto DestSrc = TII->isCopyInstr(MI);
+  auto DestSrc = TII->isCopyLikeInstr(MI);
   if (!DestSrc)
     return;
 
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 50cbd3672fbd0d..ebae0d42ff68c5 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9180,24 +9180,46 @@ void AArch64InstrInfo::buildClearRegister(Register Reg, MachineBasicBlock &MBB,
 
 std::optional<DestSourcePair>
 AArch64InstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
-
   // AArch64::ORRWrs and AArch64::ORRXrs with WZR/XZR reg
   // and zero immediate operands used as an alias for mov instruction.
-  if (MI.getOpcode() == AArch64::ORRWrs &&
-      MI.getOperand(1).getReg() == AArch64::WZR &&
-      MI.getOperand(3).getImm() == 0x0) {
+  bool OpIsORRWrs = MI.getOpcode() == AArch64::ORRWrs;
+  bool OpIsORRXrs = MI.getOpcode() == AArch64::ORRXrs;
+  if (!(OpIsORRWrs || OpIsORRXrs) || MI.getOperand(3).getImm() != 0x0)
+    return std::nullopt;
+  Register Reg1 = MI.getOperand(1).getReg();
+
+  if (OpIsORRWrs && Reg1 == AArch64::WZR) {
+    Register Reg0 = MI.getOperand(0).getReg();
+    // ORRWrs is copy instruction when there's no implicit def of the X
+    // register.
+    if (Reg0.isPhysical()) {
+      const MachineFunction *MF = MI.getMF();
+      const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
+      for (const MachineOperand &MO : MI.implicit_operands())
+        if (MO.isDef() && MO.isImplicit() &&
+            TRI->isSubRegister(MO.getReg(), Reg0)) {
+          return std::nullopt;
+        }
+    }
     return DestSourcePair{MI.getOperand(0), MI.getOperand(2)};
   }
 
-  if (MI.getOpcode() == AArch64::ORRXrs &&
-      MI.getOperand(1).getReg() == AArch64::XZR &&
-      MI.getOperand(3).getImm() == 0x0) {
+  if (OpIsORRXrs && Reg1 == AArch64::XZR) {
     return DestSourcePair{MI.getOperand(0), MI.getOperand(2)};
   }
 
   return std::nullopt;
 }
 
+std::optional<DestSourcePair>
+AArch64InstrInfo::isCopyLikeInstrImpl(const MachineInstr &MI) const {
+  if (MI.getOpcode() == AArch64::ORRWrs &&
+      MI.getOperand(1).getReg() == AArch64::WZR &&
+      MI.getOperand(3).getImm() == 0x0)
+    return DestSourcePair{MI.getOperand(0), MI.getOperand(2)};
+  return std::nullopt;
+}
+
 std::optional<RegImmPair>
 AArch64InstrInfo::isAddImmediate(const MachineInstr &MI, Register Reg) const {
   int Sign = 1;
@@ -9241,7 +9263,7 @@ static std::optional<ParamLoadedValue>
 describeORRLoadedValue(const MachineInstr &MI, Register DescribedReg,
                        const TargetInstrInfo *TII,
                        const TargetRegisterInfo *TRI) {
-  auto DestSrc = TII->isCopyInstr(MI);
+  auto DestSrc = TII->isCopyLikeInstr(MI);
   if (!DestSrc)
     return std::nullopt;
 
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index e97ff0a9758d69..6526f6740747ab 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -401,6 +401,8 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
   /// registers as machine operands.
   std::optional<DestSourcePair>
   isCopyInstrImpl(const MachineInstr &MI) const override;
+  std::optional<DestSourcePair>
+  isCopyLikeInstrImpl(const MachineInstr &MI) const override;
 
 private:
   unsigned getInstBundleLength(const MachineInstr &MI) const;
diff --git a/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
new file mode 100644
index 00000000000000..6c09f2ce7fcc1a
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/machine-cp-sub-reg.mir
@@ -0,0 +1,33 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
+# RUN: llc -o - %s -O3 --run-pass=machine-cp -mcp-use-is-copy-instr -mtriple=arm64-apple-macos -mcpu=apple-m1 --verify-machineinstrs | FileCheck %s
+
+---
+name: test
+tracksRegLiveness: true
+body:             |
+  ; CHECK-LABEL: name: test
+  ; CHECK: bb.0:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
+  ; CHECK-NEXT:   liveins: $w0
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $x8 = ORRXrs $xzr, undef $x0, 0, implicit $w0
+  ; CHECK-NEXT:   $w8 = ORRWrs $wzr, $w0, 0, implicit-def $x8
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1:
+  ; CHECK-NEXT:   liveins: $x8
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $x0 = ADDXri $x8, 1, 0
+  ; CHECK-NEXT:   RET undef $lr, implicit $x0
+  bb.0:
+    successors: %bb.1(0x80000000)
+    liveins: $w0
+
+    $x8 = ORRXrs $xzr, undef $x0, 0, implicit $w0
+    $w8 = ORRWrs $wzr, $w0, 0, implicit-def $x8
+
+  bb.1:
+    liveins: $x8
+    $x0 = ADDXri $x8, 1, 0
+
+    RET undef $lr, implicit $x0
+...

@DianQK DianQK requested a review from davemgreen December 13, 2023 15:07
Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for doing this, it looks like good stuff to me.

@DianQK DianQK force-pushed the aarch64-copy-orrwrs branch from d99d30c to 7e022bf Compare December 14, 2023 01:46
@DianQK DianQK force-pushed the aarch64-copy-orrwrs branch from 7e022bf to f245648 Compare December 14, 2023 01:48
Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This LGTM

@DianQK
Copy link
Member Author

DianQK commented Dec 14, 2023

Thanks for your help. I've learned a lot.

@DianQK DianQK merged commit 7649d22 into llvm:main Dec 14, 2023
@DianQK DianQK deleted the aarch64-copy-orrwrs branch December 14, 2023 11:20
nikic pushed a commit to rust-lang/llvm-project that referenced this pull request Dec 14, 2023
MingcongBai pushed a commit to AOSC-Tracking/llvm-project that referenced this pull request Mar 26, 2024
rastogishubham added a commit that referenced this pull request Jan 27, 2025
…124233)

The LiveDebugValues pass and the instruction selector (which calls
salvageCopySSA) need to be consistent on what they consider a copy
instruction. With #75184, the
definition of what a copy instruction is was narrowed for AArch64 to
exclude a w->x ORR and treat it as a zero-extend rather than a copy

However, to make sure LiveDebugValues still treats a w->x ORR as a copy,
the new function, isCopyLikeInstr was created. We need to make sure that
salvageCopySSA also calls that function.

This patch addresses this mismatch.
rastogishubham added a commit to rastogishubham/llvm-project that referenced this pull request Jan 27, 2025
…lvm#124233)

The LiveDebugValues pass and the instruction selector (which calls
salvageCopySSA) need to be consistent on what they consider a copy
instruction. With llvm#75184, the
definition of what a copy instruction is was narrowed for AArch64 to
exclude a w->x ORR and treat it as a zero-extend rather than a copy

However, to make sure LiveDebugValues still treats a w->x ORR as a copy,
the new function, isCopyLikeInstr was created. We need to make sure that
salvageCopySSA also calls that function.

This patch addresses this mismatch.

(cherry picked from commit 44c9e46)
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 27, 2025
…eCopySSA (#124233)

The LiveDebugValues pass and the instruction selector (which calls
salvageCopySSA) need to be consistent on what they consider a copy
instruction. With llvm/llvm-project#75184, the
definition of what a copy instruction is was narrowed for AArch64 to
exclude a w->x ORR and treat it as a zero-extend rather than a copy

However, to make sure LiveDebugValues still treats a w->x ORR as a copy,
the new function, isCopyLikeInstr was created. We need to make sure that
salvageCopySSA also calls that function.

This patch addresses this mismatch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong register used on apple-m1
3 participants